Introduce Functions\stubWpUrlFunctions() helper #129
Conversation
It makes use of a new UrlsHelper class. Tests and documentation added. See #125
There was a problem hiding this comment.
I've not completely reviewed this PR yet as I believe this remark needs to be addressed/hashed out first:
It looks like most of these don't take passed parameters to the original function into account, while WP does support passing parameters.
Stubbing these in the framework without taking the passes parameters into account will probably be confusing and lead to support overhead or to people just not using these stubs as they don't do what they expect.
As addressing that will require some rework in the PR (if it will be addressed), I figured my time would be better spend reviewing after the discussion has been had about the above.
| @@ -0,0 +1,125 @@ | |||
| <?php | |||
There was a problem hiding this comment.
All @param declarations in this file are missing the type information, which makes them kind of useless.
There was a problem hiding this comment.
All @param are mixed anyway. The type is checked inside the function via is_string and similar. So you can pass whatever. Mixed is implicit so to me no explicit type would be fine.
If we would add Psalm or similar, as soon I would declare @param string and then do is_string() checks inside the function Psalm would rightfully complain for "redundant condition" and "doc bloc type contradiction" (see an example).
I prefer to keep the check at runtime so all the parameters are mixed, and IMO there's no point in typing "mixed" all over the place.
Still, I like to keep the doc block for "visual help". I personally find it hard to digest a wall of code without some "calming" doc block in between functions.
Anyway, I added mixed pretty much everywhere, unless a private method where the type of the parameters is ensured.
| 'home_url' => $helper->stubUrlCallback(), | ||
| 'get_home_url' => $helper->stubUrlForSiteCallback(), | ||
| 'site_url' => $helper->stubUrlCallback(), | ||
| 'get_site_url' => $helper->stubUrlForSiteCallback(), | ||
| 'admin_url' => $helper->stubUrlCallback('wp-admin', 'admin'), | ||
| 'get_admin_url' => $helper->stubUrlForSiteCallback('wp-admin', 'admin'), | ||
| 'content_url' => $helper->stubUrlCallback('wp-content', null, false), | ||
| 'rest_url' => $helper->stubUrlCallback('wp-json'), | ||
| 'get_rest_url' => $helper->stubUrlForSiteCallback('wp-json'), | ||
| 'includes_url' => $helper->stubUrlCallback('wp-includes'), | ||
| 'network_home_url' => $helper->stubUrlCallback(), | ||
| 'network_site_url' => $helper->stubUrlCallback(), | ||
| 'network_admin_url' => $helper->stubUrlCallback('wp-admin/network', 'admin'), | ||
| 'user_admin_url' => $helper->stubUrlCallback('wp-admin/user', 'admin'), | ||
| 'wp_login_url' => static function ($redirect = '', $force_reauth = false) use ($helper) { |
There was a problem hiding this comment.
It looks like most of these don't take passed parameters to the original function into account, while WP does support passing parameters.
Stubbing these in the framework without taking the passes parameters into account will probably be confusing and lead to support overhead or to people just not using these stubs as they don't do what they expect.
There was a problem hiding this comment.
@jrfnl I think you need to look at it again :)
$helper->stubUrlCallback() and $helper->stubUrlForSiteCallback() return callbacks that take respectively 2 ($path, $schema) and 3 parameters ($blog_id, $path, $schema), that are the same core functions take.
There are 2 exceptions: content_url() and wp_login_url().
wp_login_url() takes still 2 arguments, but different from the others, and that is why it is stubbed differently.
content_url() takes 1 parameter: $path. That is why stubUrlCallback() accepts a 3rd parameter $use_schema_arg that defaults to true, but it is passed as false for content_url().
To be noted:
- the
$blog_idparameter for functions that accepts it is just ignored in stubbed functions because the base URL is taken from what is passed to thestubWpUrlFunctions()API function. - When
$schemaparam is something like'admin'or'login'it is taken into account only if HTTPs is not forced (passingtrueasstubWpUrlFunctions()'s 2nd parameter) andforce_ssl_admin()is available (likely because stubbed separately). rest_url()andget_rest_url()use'rest'as scheme. That is not relevant for the generation of the URL (in WP just like in the stubbed version), it is used in WP only as the 3rd parameter,$orig_scheme, passed to the'set_url_scheme'hook, something that I think is pretty safe to ignore for the stubbed version of the function.
TL;DR: all the stubbed functions accept the same parameters and work very similarly to the WP counterparts. For some edge cases, it might be needed to stub is_ssl() and/or force_ssl_admin() separately.
You can even see the parameters used with success in tests: https://github.com/Brain-WP/BrainMonkey/blob/feature/stub-wp-urls/tests/cases/unit/Api/FunctionsTest.php#L439-L455
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Co-authored-by: Juliette <[email protected]>
Codecov ReportPatch coverage:
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## master #129 +/- ##
============================================
- Coverage 92.25% 92.23% -0.02%
- Complexity 312 342 +30
============================================
Files 28 29 +1
Lines 710 876 +166
============================================
+ Hits 655 808 +153
- Misses 55 68 +13
... and 20 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
Call \is_ssl() and \force_ssl_admin explicitly if stubbed. We expect those to be stubbed in the root namespace.
Commented all your points. Your concerns about parameters, as better explained below, aren't correct because parameters are accepted in the same way WP does. Besides my reasoning in other comments, there're tests to prove it :) So, please @jrfnl, review it again when/if you have time. |
It makes use of a new UrlsHelper class.
Tests and documentation added.
See #125